Skip to content

Harden SecurePasswords assignment input/output handling and remove per-request Zxcvbn initialization#13

Draft
Giuseppe Guerra (GGuerraReply) with Copilot wants to merge 2 commits into
mainfrom
copilot/add-password-input-validation
Draft

Harden SecurePasswords assignment input/output handling and remove per-request Zxcvbn initialization#13
Giuseppe Guerra (GGuerraReply) with Copilot wants to merge 2 commits into
mainfrom
copilot/add-password-input-validation

Conversation

Copilot AI commented Apr 22, 2026 •
edited
Loading

Copy link
Copy Markdown

This PR addresses four issues in SecurePasswordsAssignment: missing password input validation, repeated Zxcvbn construction per request, unescaped dynamic HTML output, and a misleading modulo expression in calculateTime.

  • Input validation

    • Added a null/blank guard for password at the start of completed(...).
    • Returns a failed AttackResult with securepassword-failed feedback and a clear error message when input is empty.

  • Zxcvbn lifecycle

    • Replaced per-request new Zxcvbn() with a class-level private static final Zxcvbn ZXCVBN.
    • Updated strength measurement to use the shared instance.

  • Output encoding / XSS hardening

    • Added HtmlUtils and escaped dynamic values before appending to HTML output:
      • password length
      • guesses
      • score
      • calculated crack time
      • warning text
      • feedback suggestions
    • Keeps rendered HTML structure while preventing untrusted content from being interpreted as markup.

  • Time calculation clarity

    • Simplified long sec = (seconds % min * s); to long sec = seconds % min; to remove precedence ambiguity and dead multiply-by-1 logic.

  • Focused regression coverage

    • Added SecurePasswordsAssignmentTest for:
      • null/blank password rejection
      • masked password output behavior
      • calculateTime(61) expected remainder formatting
if (password == null || password.trim().isEmpty()) {
  return failed(this)
      .feedback("securepassword-failed")
      .output("<b>Error:</b> Password must not be empty.")
      .build();
}

private static final Zxcvbn ZXCVBN = new Zxcvbn();
Strength strength = ZXCVBN.measure(password);

Warning

Firewall rules blocked me from connecting to one or more addresses (expand for details)

I tried to connect to the following addresses, but was blocked by firewall rules:

  • checkstyle.org
    • Triggering command: /opt/hostedtoolcache/CodeQL/2.25.1/x64/codeql/tools/linux64/java/bin/java /opt/hostedtoolcache/CodeQL/2.25.1/x64/codeql/tools/linux64/java/bin/java -jar /opt/hostedtoolcache/CodeQL/2.25.1/x64/codeql/xml/tools/xml-extractor.jar --fileList=/tmp/codeql-scratch-dbf01dac03ae7d91/dbs/java/working/files-to-index2027385754433674253.list --sourceArchiveDir=/tmp/codeql-scratch-dbf01dac03ae7d91/dbs/java/src --outputDir=/tmp/codeql-scratch-dbf01dac03ae7d91/dbs/java/trap/java (dns block)
  • checkstyle.sourceforge.net
    • Triggering command: /opt/hostedtoolcache/CodeQL/2.25.1/x64/codeql/tools/linux64/java/bin/java /opt/hostedtoolcache/CodeQL/2.25.1/x64/codeql/tools/linux64/java/bin/java -jar /opt/hostedtoolcache/CodeQL/2.25.1/x64/codeql/xml/tools/xml-extractor.jar --fileList=/tmp/codeql-scratch-dbf01dac03ae7d91/dbs/java/working/files-to-index2027385754433674253.list --sourceArchiveDir=/tmp/codeql-scratch-dbf01dac03ae7d91/dbs/java/src --outputDir=/tmp/codeql-scratch-dbf01dac03ae7d91/dbs/java/trap/java (dns block)

If you need me to access, download, or install something from one of these locations, you can either:

Original prompt 
Please apply the following diffs and create a pull request.
Once the PR is ready, give it a title based on the messages of the fixes being applied.

[{"message":"Missing input validation: the password parameter is not validated for null or empty values before being processed. Add validation to check if the password is null or empty and return an appropriate error response.","fixFiles":[{"filePath":"src/main/java/org/owasp/webgoat/lessons/securepasswords/SecurePasswordsAssignment.java","diff":"diff --git a/src/main/java/org/owasp/webgoat/lessons/securepasswords/SecurePasswordsAssignment.java b/src/main/java/org/owasp/webgoat/lessons/securepasswords/SecurePasswordsAssignment.java\n--- a/src/main/java/org/owasp/webgoat/lessons/securepasswords/SecurePasswordsAssignment.java\n+++ b/src/main/java/org/owasp/webgoat/lessons/securepasswords/SecurePasswordsAssignment.java\n@@ -25,6 +25,13 @@\n   @PostMapping(\"SecurePasswords/assignment\")\n   @ResponseBody\n   public AttackResult completed(@RequestParam String password) {\n+    if (password == null || password.trim().isEmpty()) {\n+      return failed(this)\n+          .feedback(\"securepassword-failed\")\n+          .output(\"<b>Error:</b> Password must not be empty.\")\n+          .build();\n+    }\n+\n     Zxcvbn zxcvbn = new Zxcvbn();\n     StringBuilder output = new StringBuilder();\n     DecimalFormat df = new DecimalFormat(\"0\", DecimalFormatSymbols.getInstance(Locale.ENGLISH));\n"}]},{"message":"A new Zxcvbn instance is created on every request. The Zxcvbn class loads frequency dictionaries and pattern matchers which can be resource-intensive. Consider making this a class-level field or singleton to avoid repeated initialization overhead.","fixFiles":[{"filePath":"src/main/java/org/owasp/webgoat/lessons/securepasswords/SecurePasswordsAssignment.java","diff":"diff --git a/src/main/java/org/owasp/webgoat/lessons/securepasswords/SecurePasswordsAssignment.java b/src/main/java/org/owasp/webgoat/lessons/securepasswords/SecurePasswordsAssignment.java\n--- a/src/main/java/org/owasp/webgoat/lessons/securepasswords/SecurePasswordsAssignment.java\n+++ b/src/main/java/org/owasp/webgoat/lessons/securepasswords/SecurePasswordsAssignment.java\n@@ -22,14 +22,15 @@\n @RestController\n public class SecurePasswordsAssignment implements AssignmentEndpoint {\n \n+  private static final Zxcvbn ZXCVBN = new Zxcvbn();\n+\n   @PostMapping(\"SecurePasswords/assignment\")\n   @ResponseBody\n   public AttackResult completed(@RequestParam String password) {\n-    Zxcvbn zxcvbn = new Zxcvbn();\n     StringBuilder output = new StringBuilder();\n     DecimalFormat df = new DecimalFormat(\"0\", DecimalFormatSymbols.getInstance(Locale.ENGLISH));\n     df.setMaximumFractionDigits(340);\n-    Strength strength = zxcvbn.measure(password);\n+    Strength strength = ZXCVBN.measure(password);\n \n     output.append(\"<b>Your Password: *******</b></br>\");\n     output.append(\"<b>Length: </b>\" + password.length() + \"</br>\");\n"}]},{"message":"HTML output is constructed using string concatenation without proper escaping. If the password contains special HTML characters, they could be interpreted as markup. While line 34 masks the password, the length and other feedback fields could potentially expose user data. Use proper HTML escaping or a templating engine to prevent XSS vulnerabilities.","fixFiles":[{"filePath":"src/main/java/org/owasp/webgoat/lessons/securepasswords/SecurePasswordsAssignment.java","diff":"diff --git a/src/main/java/org/owasp/webgoat/lessons/securepasswords/SecurePasswordsAssignment.java b/src/main/java/org/owasp/webgoat/lessons/securepasswords/SecurePasswordsAssignment.java\n--- a/src/main/java/org/owasp/webgoat/lessons/securepasswords/SecurePasswordsAssignment.java\n+++ b/src/main/java/org/owasp/webgoat/lessons/securepasswords/SecurePasswordsAssignment.java\n@@ -18,6 +18,7 @@\n import org.springframework.web.bind.annotation.RequestParam;\n import org.springframework.web.bind.annotation.ResponseBody;\n import org.springframework.web.bind.annotation.RestController;\n+import org.springframework.web.util.HtmlUtils;\n \n @RestController\n public class SecurePasswordsAssignment implements AssignmentEndpoint {\n@@ -32,14 +33,14 @@\n     Strength strength = zxcvbn.measure(password);\n \n     output.append(\"<b>Your Password: *******</b></br>\");\n-    output.append(\"<b>Length: </b>\" + password.length() + \"</br>\");\n+    output.append(\"<b>Length: </b>\" + HtmlUtils.htmlEscape(String.valueOf(password.length())) + \"</br>\");\n     output.append(\n         \"<b>Estimated guesses needed to crack your password: </b>\"\n-            + df.format(strength.getGuesses())\n+            + HtmlUtils.htmlEscape(df.format(strength.getGuesses()))\n             + \"</br>\");\n     output.append(\n         \"<div style=\\\"float: left;padding-right: 10px;\\\"><b>Score: </b>\"\n-            + strength.getScore()\n+            + HtmlUtils.htmlEscape(String.valueOf(strength.getSco...

Copilot AI changed the title [WIP] Add input validation for password parameter Harden SecurePasswords assignment input/output handling and remove per-request Zxcvbn initialization Apr 22, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@GGuerraReply Giuseppe Guerra (GGuerraReply) Awaiting requested review from GGuerraReply
Copilot code review Copilot Awaiting requested review from Copilot Copilot will automatically review once the pull request is marked ready for review

Assignees

@GGuerraReply Giuseppe Guerra (GGuerraReply)

Copilot code review Copilot

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@GGuerraReply